Fixing prefix to s3Origin...

related #147

I have already deployed a an environment using this new version and it works fine here.

Nice work! Interesting... you didn't have to set basePath in next.config.js?

@khuezy yes, they work together, you set that on the next.config.js so the next lambda server knows how to handle it, than you can set it on the Nextjs deployment to put it under that basePath on cloudfront and s3

I didn't see a next.config.js in the PR

because the examples use open-next git submodule I cannot change it

BTW, you also need to set assetPrefix: '/...',with the same value as the basePath for it to work

I see. None of the open-next examples router apps have basePath configured. I see you have examples/multiple-sites but didn't see where the stack was referencing.

to validate it you would need to manually edit the referenced modules

@onhate, thanks so much for contributing this feature! This feature brings a lot more flexibility to this construct. Let me test this out and get back to you.

@onhate, great job on this feature. I've made some simplifications (IMO), moved basePath out of NextjsBaseProps to NextjsProps since I'll be removing that soon to make props more explicit per construct, and added the distribution prop to NextjsProps instead of via defaults since that might be removed in the future soon too. I think those props are significant enough that they should be top level.
I tried to create a new branch in your repo to create a PR so you can easily see my changes but i got access denied. Could you give me access?

nice! I've just added you @bestickley

@bestickley the part of get/create distribution got simpler indeed using just the addBehavior. My only concern is that now we have two options of passing 'distribution' (Distribution and OverwriteConfig) and Distribution has higher precedence than config, maybe a warning?

@onhate, great point. With that said, I still don't think it makes sense to keep them as the same prop. Do you think I should log a warning or throw an error? Logged warning could be missed, error will make sure user sees it. There is no reason to customize distribution when providing distribution. It will have no affect.

well, passing an overwrite when passing a distribution could lead to think the passed distribution can be 'customized', so an error would prevent this thought, I'll add this check here.

@onhate, agreed. I've added it my PR.

we both did it 🤣

Haha, yours looks good to me. Ok, so I've deployed but I'm getting a 404. Could you check it out?
https://d1s4ya2t04oexs.cloudfront.net/app-router

@bestickley checking, I already have a deployment working with 2 next.js sites and a home page, it can be something on the application, I'll return with the findings

@onhate, another test case I tried is specifying basePath and no distribution. When I do that I get:

7:22:55 AM | CREATE_FAILED        | AWS::CloudFront::Distribution                   | pagesrouterDistribution428530EB
Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: The parameter path pattern must be unique. (Service: CloudFront, Status Code: 400, Request ID: 6edac
785-8957-4521-9b08-b3335daf7775)" (RequestToken: 6c0846e4-77fa-496d-85f9-f56fd532ba2e, HandlerErrorCode: InvalidRequest)

It could just be me though. Do you mind trying as well?

sure, checking this too, I may know what it is

a typo on the refactoring :) fixing

@onhate, my bad! Thank you :)

https://d1nlmfis3ufm0d.cloudfront.net/pages-router
https://d1nlmfis3ufm0d.cloudfront.net/app-router
https://d1nlmfis3ufm0d.cloudfront.net

it works

try to run an Invalidation on your cloudfront to validate if the 404 goes away

@onhate, I have deployed most recent version and invalidated CloudFront. I'm getting 404 on pages-router of multi site example here and CSS issues on high security example here.

Also, I'm seeing CSS isues on your examples above.

@bestickley that's because the application needs to be ready to load assets from a prefix path, as you can see the code examples they all load from / directly instead of /prefix, if you run the apps in dev mode with assetsPrefix and basePath they will also fail. In summary, your app needs to developed to run under a basePath.

Regarding the 404 that's weird as my deployment just works, could you share a printscreen of the CloudFront behaviors configuration page?

hmmm, wait, the css issue was supposed to not happen as I also "wrap" the assets folder under the prefix basePath, let me check.

the /app-route/albums is loading fine on my deployment

ah, the only difference from your deployments and mine is that you are not passing a custom distribution right? In this case it will create a default route (catch all) pointing to the lambda, but the lambda was built to run under a basePath so the default route will always return 404, let me try to debug deploying without passing a custom distribution and using basePath.

@onhate, sorry not CSS, but images of your app router:

@bestickley Like I thought, the app needs to be developed with /subpath in mind, if you see the code that adds the image on the screen, https://github.com/sst/open-next/blob/main/examples/shared/components/Nav/index.tsx#L12 it is just assuming the image is on /static, if you run app-routes locally in dev mode with basePath and assetPrefix it will fail the same way to load the images.

@onhate, that makes sense. issue resolved.
Pending issues to resolve before merging is to figure out why my pages-router isn't working and why high security version isn't working.

https://djaxa5d84nqeo.cloudfront.net/high-security high security is working here, same issues with the images but ok, regarding your 404, could you make sure the basePath and assetPrefix you had built the example app are matching with the configured basePath on the CDK construct? I had a similar issue when tried to deploy on /high-security I forgot to change the basePath that was /app-router on the next.config.js and it was returning 404.

double check because high-security example has skipBuild: false, so even if you change on next.config.js it will not rebuild

@onhate, assetPrefix does fix the images. You just need to ensure trailing slash. See this blog. Both my multiple sites example and high security example are working now!
One last thing before merging to main. Could run e2e tests on multiple sites example for app and pages to ensure all proper functionality it working?

that's good news :)

well, I have a production environment that I migrated from subdomains do /subpath already running, if you want to see it in action we can talk in private and I can show you, but unfortunately I cannot share the url (yet)

Update: I was wrong about assetPrefix working for public files (images). Block quote below from here explains:

Files in the public folder; if you want to serve those assets over a CDN, you'll have to introduce the prefix yourself

Glad to hear that you have a production environment with it working. There are still likely features your environment is not testing that e2e tests will test. However, the e2e tests we use that are in open-next git submodule do not accomodate basePath customization and I don't have enough time now to fix all of the e2e tests. I'm going to create an issue for a future PR that pulls the Next.js example apps from open-next/examples into our examples folder and pull the e2e tests in. This way we won't have to manually update open-next/examples app to test out multi site example.

One last thing, could you update the TS Doc for basePath to the below on both NextjsProps and NexjsDistributionProps? Notice the explanation about basePath in next.config.ts.

/**
   * Optional value to prefix the Next.js site under a /prefix path on CloudFront.
   * Usually used when you deploy multiple Next.js sites on same domain using /sub-path
   *
   * Note, you'll need to set [basePath](https://nextjs.org/docs/app/api-reference/next-config-js/basePath)
   * in your `next.config.ts` to this value and ensure any files in `public`
   * folder have correct prefix.
   * @example "/my-base-path"
   */
readonly basePath?: string;

@bestickley sorry, I'm not following, how should I run e2e? by your comment are we going to do it on another PR? As this change is new and optional I think we could move on with this PR and work incrementally on e2e on new PRs.

Docs updated.

See #151. We'll create them in another PR. If you wanted to support that effort, it would be greatly appreciated. Agreed. Thank you!

@bestickley sure thing, I'll help on it

Merged. Thank you @onhate for your hard work on this feature!